Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: pickle shelffile -> standard_address_space.sql #763

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

brubbel
Copy link
Contributor

@brubbel brubbel commented Nov 27, 2018

Refactoring the shelffile from a monolithic blob to sqlite3 db.
Advantages over pickle:

  • Fast
  • Small (25% of fill_address_space code)
  • Not dependent on Python2/3 version
  • Supports transactional read/(write TODO)
  • Supports real-time persistence of user address-space (TODO)
  • Strong typed: only INTEGER, TEXT and opc-ua to_binary() BLOB
  • Drop-in replacement for memory-based AddressSpace

Built-in integrity check on generate_address_space.py dump.

Usage: shelffile=True when starting server.

@zerox1212
Copy link
Contributor

zerox1212 commented Nov 27, 2018

Interesting work.

  1. How difficult to support more than NumericNodeID? This will definitely be required to persist a user address space (which would be a great feature). In the past I always had to save/restore the binary file.
  2. I'm not sure it will be as fast as the shelf, but should be good enough.
  3. Update gitignore for the database file.

@oroulet
Copy link
Member

oroulet commented Nov 27, 2018

What is the point of that change? The shelves stuff was just a ugly hack for very slow systems. Btw sqlite is not typed at all. You store whatever you want whereever you want.

@brubbel
Copy link
Contributor Author

brubbel commented Nov 27, 2018

@zerox1212

  1. Not very difficult. Numeric nodeid's are used as key for lookups, but we can store the original NodeId alongside. I first started with the standard address space because it provides a good test set.
    There is some more work to do, as my goal is to have 2 files: standard_address_space.sql (read-only) and shelffile.sql (r/w) for all namespaces > 0.

@oroulet
I have those slow systems (rpi2), and the boot time is really annoying because of those 30k node attributes restored on boot :-)
Then I started using the shelffile and it became incompatible after a python update. Ok, for the standard address space you can indeed regenerate it.

Also, writing user aspace to a pickle file is not an option for the following reasons:

  • User address space is dynamic, varying nodes with varying parameters. Not known at boot time, device node tree is built over time based on incoming measurements.
  • System is unplugged without warning. A pickle file gets corrupt. I have never seen an SQLite database in WAL mode to become fully corrupt. You may loose some data since the last checkpoint, but that is not a disaster.

Please comment on any issue! both implementation and idea's.

@zerox1212
Copy link
Contributor

zerox1212 commented Nov 27, 2018

I'm in support of something that we can build into address space persistence.
It reduces the following which I have had in my use cases:

  1. Do not need to rebuild starting address space from code or waste time parsing XML every time server starts.
  2. Do not need to be sure that server shutdowns in order to export address space as XML or dump binary in order to restore as mentioned in point 1.
  3. Can use UA address space to save data like parameters, HMI settings, etc. that would otherwise have to be handled in two spots (like when UA is "giving access" to an underlying system). This is especially useful if you create custom "drivers" for UA like Modbus TCP where some configuration data needs to be persisted. In other words it reduces code duplication in some applications.

My only other input is that if we really go this direction it might be better to create an interface like history has. That way end user could potentially implement their own persistence layer.

@brubbel
Copy link
Contributor Author

brubbel commented Nov 27, 2018

I am somewhat concerned about the divergence between the main branch and the parallel work that is done on the asyncio-server. I really would be able to do await, given that Python2 end-of-life is not so far in the future. Or is there already an official asyncio-release on pypy?

@brubbel
Copy link
Contributor Author

brubbel commented Nov 27, 2018

@zerox1212
Yes, then why not merge the user-space shelf in the history.sql?
So you have all data at one place. Then just on-the-fly fetch the latest value from history at request of that node. No need to store that latest value somewhere else or restore all data on server start.

My only other input is that if we really go this direction it might be better to create an interface like history has.

That is a very interesting remark.

@zerox1212
Copy link
Contributor

I think history is better done separately. The OPC UA spec defines how history works in detail.

If we are strictly keeping to the spec I do not know if it says anything about persisting server state.

In the end I was just giving some inputs. I am not able to do much work on this library myself. However I can review some stuff about this topic because I was one of the people working on shelf, dump, and load because I needed to restore server state. I think in the end I actually used XML export/import for persistence.

@brubbel
Copy link
Contributor Author

brubbel commented Nov 27, 2018

Maybe I'm wrong, but why not (optionally) storing user aspace in the same sql file as history?
In a separate sql-table of course. No need to change how history works.
So that 1 file has all related data.

@zerox1212
Copy link
Contributor

zerox1212 commented Nov 27, 2018

Because history SQL implementation is rather simplistic and is not a good model to build on. It makes one table per historized node. You would not want this for potentially thousands of nodes. Only nodes with history require this design.

Plus if node names change or other such things data is orphaned in the database because there is not really a mechanism to remove data.

@oroulet
Copy link
Member

oroulet commented Nov 28, 2018

I have never seen an SQLite database in WAL mode to become fully corrupt.
I have not see it myself , bu I have heard many people complain about easy corruption in sqlite.

opcua-asyncio is a python 3.6 only fork so we do not care about python2 there. I think the approach is correct but yest I am concerned about divergence, that is why I thing, large restructure work should wait for that work to stabilize. better work on improving opcua-asyncio than python-opcua

concerning the shelve stuff I thing the people wanting to save things in the opcua database have a fundamental misunderstanding of what opcua is. opua is a communication protocol, saving things inside the communication protocol does not make sense at all, the underlying system should save its state, opcua is just here to expose data from the underlying system. I am wondering if I made some errors in the API that make people think they should do that kind of things....

but I agree that using a sqlite db to store the standard address space might have an interest, this standard address space slows down starting time and uses memory for something that is ignored 99.99 % of the time. But onece again this should be something to implement in asyncio-opcua

@brubbel if you have some time or want to learn some modern python things, then look at the fork, try it and try to improve it. We also need to make a sync wrapper around it, there are a lot of things to things about here. probably make some classes with decorator and maybe the async Node should be called AsyncNode and the syn node Node to stay compatible with python-opcua... or just accept that we not completly API compatible anymore

@brubbel
Copy link
Contributor Author

brubbel commented Nov 28, 2018

I agree that opcua-ua is not meant as storage space. However, It is my opinion that the library must provide a simple means to override some parts such as the address space. With that in mind I think it is a good idea to support a strategy for that.
For example: address space must a dict-compatible + get_attribute_value + set_attribute_value etc.

I would like to start using the opcua-asyncio for the following reasons:

  • There are some nice modifications: __aenter__ instead of start and __aexit__ for stop.
  • In the current version there is no way for the user to use an async history and async address space.
    Synchronous I/O really kills performance in my case.
  • User may want to provide its own asyncio thread, instead of relying on the one that is started in the background.

However:

  • I don't have much time, as all of you. So I really appreciate your views on the best path.
  • In favor of a working version at any time :-) Breaking API's is not a concern. Bugs are. With that in mind: do you consider the asyncio branch stable enough?

One other remark:
Debian stable is still on Python3.5 if I'm correct. Are there any 3.6-only things used? For me that would be a reason to stay with the python-opcua version.

@oroulet
Copy link
Member

oroulet commented Nov 28, 2018

It is my opinion that the library must provide a simple means to override some parts such as the address space

I agree that this could be an improvment, if you do that you need to provide some API to notify the system for change in values so datachange events can be generated. Such a change will require quite a lot fo work

do you consider the asyncio branch stable enough?

I have no idea 😢 I haven't had time to try it t but I looked at the changes and they all looked correct,
@cbergmiller what is the status and plan currently
Ideally we should first work on the sync API so tests can be run without having to modify all of them.

@zerox1212
Copy link
Contributor

zerox1212 commented Nov 28, 2018

@oroulet said OPC UA is not really meant for persistence (except history I guess...). For me it's still a valid request to want persistence of user nodes, mainly because it can make prototyping UA software with this library faster. In other words I'm lazy and I don't want to write a persisting data layer and then create a UA address space that will basically be the exact same thing. Maybe my use case is just too special.

An address space design that could can be overridden would be a nice option so user like me can break the rules. :)

@brubbel
Copy link
Contributor Author

brubbel commented Nov 30, 2018

So here it is, based on your comments:
Instead of providing a shelffile, you can now provide your own address space. (If you don't provide it, InternalServer reverts to rebuilding the standard address space from scratch.)

with AddressSpace() as myAspace:
    server = Server(aspace=myAspace)

Of course this address space is empty, but you can load from sqlite:

with AddressSpaceSQLite(sqlFile='/path/to/standard_address_space.sql') as myAspace:
    server = Server(aspace=myAspace)

Now you can start to populate the address space with your own nodes, as usual.
Then you can simply dump your own address space, without stopping the server:

with AddressSpaceSQLite(sqlFile='/path/to/standard_address_space.sql') as myAspace:
    server = Server(aspace=myAspace)
    <...>
    with AddressSpaceSQLite(cache=myAspace, sqlFile=path) as aspace_sql:
        aspace_sql.dump(namespaceidx=2)

(Don't use dump() regularly however, it is slow. Still need to implement on-the-fly writes)
And to load, simply stack it on top of the standard address space:

with AddressSpaceSQLite(sqlFile='/path/to/standard_address_space.sql') as stdAspace, \
  AddressSpaceSQLite(sqlFile='/path/to/my_address_space.sql', cache = stdAspace) as myAspace:
    server = Server(aspace=myAspace)

@oroulet said:

if you do that you need to provide some API to notify the system for change in values so
datachange events can be generated. Such a change will require quite a lot fo work

In the proposed implementation, you still read and write to the server via the usual set/get_value API, so all is fine.

@zerox1212 said

An address space design that could can be overridden would be a nice option so user like
me can break the rules. :)

Just inherit from AddressSpace, which is basically a threadsafe dict and you're good to go.
AddressSpaceSQLite can be used as a reference implementation.

The current AddressSpaceSQLite implementation still requires a aspace.dump() to save the address space to disk. However, the path is now open to implement on-the-fly writes to sqlite3.

@oroulet
Copy link
Member

oroulet commented Nov 30, 2018

I haven't looked at this closely yet, but one more thing is that you should be really carefull with performance, this is an arear which we had to really optimized. I see that your replaced several dicts access with functions, this might slow downn things quite a lot for example. It was also optimized many places to avoid searching dicts.
so basically you will need to setup some performance comparison between your approach and the old one.

@brubbel
Copy link
Contributor Author

brubbel commented Nov 30, 2018

True, the first read will be slow as data is fetched from disk. After that there should be no difference.
I will find out soon enough when it is put at real work 👍
What I'm still considering is a way to solve searches for non-existent nodes in the aspace (BadNodeIdUnknown), since there would be a repetitive fallback to (slow) database.

Also, the current implementation is synchronous and the async-opcua could really shine here as you can continue serving/processing other tasks while waiting for IO, or you could even restore cache during idle time.

@brubbel
Copy link
Contributor Author

brubbel commented Nov 30, 2018

Note to self: generate_nodeid may be a problem. (Did it ever work with the shelf? It looks only in memory if I'm correct.)
Better/faster to generate a random id and check if it is in the address space?

# get the biggest identifier number from the existed nodes in address space
identifier_list = sorted([nodeid.Identifier for nodeid in self._nodes.keys()
if nodeid.NamespaceIndex == idx and nodeid.NodeIdType
in (ua.NodeIdType.Numeric, ua.NodeIdType.TwoByte, ua.NodeIdType.FourByte)])

@brubbel
Copy link
Contributor Author

brubbel commented Dec 5, 2018

TL;DR; address space .dump() is now obsolete :-) real-time updates of database/backend.

Usage:

with SQLite3Backend(sqlFile='/path/to/standard_address_space.sql') as stdBackend, \
       SQLite3Backend(sqlFile='my_address_space.sql', readonly=False) as myBackend, \
       AddressSpaceSQLite(backend=stdBackend) as stdAspace, \
       AddressSpaceSQLite(backend=myBackend, cache=stdAspace) as myAspace:
    assert(isinstance(myAspace, AddressSpace)) # <- interface like HistoryStorageInterface
    server = Server(aspace=myAspace)
    <...>

How it works:

  1. stdBackend provides readonly access to the standard address space on disk.
  2. myBackend provides r/w access to your custom address space on disk.
  3. stdAspace reverts to stdBackend for nodes not in cache.
  4. myAspace reverts to myBackend for nodes not in (stdAspace, cache)
  5. Server uses myAspace as write-through cache.

Data becomes persistent by:

  • automatic checkpoints every 90sec, if Server_ServerStatus_CurrentTime ticks are active (default)
  • or by calling myBackend.wal_checkpoint() manually
  • or by a clean shutdown.

Advantages.

  • Share database with history.sql becomes possible. Or not, whatever you want.
  • (TODO) provide your custom backend, may even retrieve data from network etc.

Speed
Around 10k record updates/sec on my i3-3227U, when multiple statements in a single transaction.
So if you need high-speed updates, override the execute_read method to bypass the automatic commits and bundle writes in a single transaction, or set a limit on the number of updates per time.

@zerox1212
Copy link
Contributor

zerox1212 commented Dec 5, 2018

Looks nice. Let's wait for @oroulet to review.

If you don't define aspace everything works as normal, correct?

Only small points I see are that the x.sql files should not be committed to the repo and a usage example could be nice.

@brubbel
Copy link
Contributor Author

brubbel commented Dec 5, 2018

Correct, everything remains as before when the user does not define aspace. That means no sqlite involved and standard address space re-generated at startup.
(I removed all shelffile related code to avoid confusion)

Only the pre-generated standard_address_space.sql is added to the repo.

My idea is to let the [WIP] hanging for a while until I've figured out most of the probable ;-) bugs and optimizations, but please try and comment.

@brubbel
Copy link
Contributor Author

brubbel commented Dec 5, 2018

Added a shortcut for loading the standard address space.

with SQLite3Backend(sqlFile='my_address_space.sql', readonly=False) as myBackend, \
       StandardAddressSpaceSQLite() as stdAspace, \
       AddressSpaceSQLite(backend=myBackend, cache=stdAspace) as myAspace:
    assert(isinstance(myAspace, AddressSpace)) # <- interface like HistoryStorageInterface
    server = Server(aspace=myAspace)
    <...>

@brubbel
Copy link
Contributor Author

brubbel commented Dec 6, 2018

Added examples/server-persistent.py

  • Open en close the server multiple times to fill my_address_space.sql
  • Write values from your client into the server and see that they are also persisted.

@oroulet
Copy link
Member

oroulet commented Dec 7, 2018

The idea of putting the standard address space in a sqlite db is not bad, that is fine, but I hada a few questions:

  • can we have persistence off by default? This is really against the idea of a protocol,
  • how can users force update of sql address space? or how can we have the address space automatically detect the standard has changed (due to new realease or whatever) we need a version string somewhere

@brubbel
Copy link
Contributor Author

brubbel commented Dec 7, 2018

can we have persistence off by default?

The key is to set readonly=True (default) when a layer in the address space stack is initialized.
(The StandardAddressSpaceSQLite shortcut is already read-only)
Examples without persistence:

Option 1 (default): no custom address space.
server = Server(aspace=None) or server = Server()

Option 2: read-only standard address space from sql. Non-persistent cache.

with StandardAddressSpaceSQLite() as stdAspace:
    server = Server(aspace=stdAspace)

Option 3: read-only std aspace + read-only custom aspace. Non-persistent cache.

with SQLite3Backend(sqlFile='my_address_space.py', readonly=True) as backend, \
    StandardAddressSpaceSQLite() as stdAspace, \
    AddressSpaceSQLite(backend=backend, cache=stdAspace) as myAspace:
server = Server(aspace=myAspace)

how can users force update of sql address space?

As is done in this PR, I would suggest to keep the standard_address_space.sql within the distribution,
in the opcua/server/standard_address_space/ directory. It is generated together with the standard_address_space_partx.py files.

The version is already embedded in the standard address space:
image
image

@oroulet
Copy link
Member

oroulet commented Dec 7, 2018

The version is already embedded in the standard address space:

so maybe you should read that value at startup and regenerate address space if current addresse space is different than the one in code?
Maybe you can generate a file code ua_std_version.py in opcua/ua/server/address_space when callinge generate_address_space.py and compare that value to the version in sql db?

@brubbel
Copy link
Contributor Author

brubbel commented Dec 7, 2018

Wouldn't that require to first load the standard_address_space_partx.py files? Which is the main reason to use standard_address_space.sql on slow devices?

Or maybe you meant not to clutter the git repository with standard_address_space.sql blob updates which are basically unchanged if the version stays the same?
I'm not entirely sure here what the goal is.

@oroulet
Copy link
Member

oroulet commented Dec 7, 2018

I may forget something, but my proposition was

if running sql
load only opcua.ua.ua_std_version.py
check that version to version in sql
load standard address space and save it to disk if sql version < opcua.ua.ua_std_version.py

@oroulet
Copy link
Member

oroulet commented Dec 7, 2018

oops I did not see that you put a huge a blob file in repository. Yes this is a very bad idea ;-).
I did not read your entire code yet and was expecting that you populate the sql db from the address space in memory... but was you idea to distribute the address space as an sql blob with python-opcua? That is not going to be very popular....
I think that you should populate the db at first startup if it does not exist or if outdated. you can also add a script to generate the blob if some people want to generate it by hand

@oroulet
Copy link
Member

oroulet commented Dec 7, 2018

as written earlier if you manage to implement that correctly I thing we should considere using sql as default to reduse startup time. But then we really need to make sure it does not give and performance drawback for high speed read_write on client and server side.

@brubbel
Copy link
Contributor Author

brubbel commented Dec 7, 2018

How would you make the link to GUID for nodes that are only on disk?
Say a remote client requests nodeid(i=123,ns=2) -> [?] -> sql lookup.

All things I have been circling around boil down to a unique identifier, or a hash thereof (which may introduce hash collisions if too short).

One needs something unique identifiable, so in the end why not use the opc-ua well-defined binary nodeid representation, preferrably numeric (if possible) to capture numeric/twobyte/fourbyte as being the same node. That is also what is (implicitly) done when calculating the python hash, because it ignores the NodeIdType.
Still open for all options though...

@zerox1212
Copy link
Contributor

Could we look for separate library that will hash in a predictable way i.e. not salt the hash?

@brubbel
Copy link
Contributor Author

brubbel commented Dec 7, 2018

Have been looking at fast hash algorithms like fnv-1a or murmur, but the only thing that they add is a risk for hash collisions and an extra dependency, and no way to calculate backwards to the original nodeid.

If you have a lot of nodes, the risk of a collision increases dramatically, unless you go for hash sizes larger than the binary representation of the nodeid, so no gain there.

32-bit hash -> 1% collision rate for 10k nodes
64-bit hash -> negligable
binary numericNodeId: 56 bit (7bytes)

@brubbel
Copy link
Contributor Author

brubbel commented Dec 7, 2018

Even worse:
32bit hash -> 50% collision rate since a node has multiple attributes (calculated with 8 attr/node*10k)
64bit hash -> 1e-10 (ok)
56bit binary numericNodeId -> 0%

@brubbel
Copy link
Contributor Author

brubbel commented Dec 7, 2018

Support for all other nodeid types other than numeric/twobyte/fourbyte.

image

@brubbel
Copy link
Contributor Author

brubbel commented Dec 8, 2018

SQLite3 needs write access to read-only standard_address_space.sql file. Added the immutable=true flag when opening the database to prevent the "Read-only file system" exception. Not supported by Python2.x, checked the source code of cPython for that.

Py2.x users can't use the wrapper:

with StandardAddressSpaceSQLite() as stdAspace:
    <...>

But will have to copy the standard_address_space.sql and do:

with SQLite3Backend(sqlFile='/path/to/standard_address_space.sql') as stdBackend, \
       AddressSpaceSQLite(backend=stdBackend) as stdAspace:
    <...>

where /path/to/standard_address_space.sql is writable and also in a directory with write access.

@brubbel
Copy link
Contributor Author

brubbel commented Dec 9, 2018

multithreading: sqlite does not allow interleaving write+commits between multiple threads.
Fixed by protecting write + commit (if required) under the same Lock().

@brubbel
Copy link
Contributor Author

brubbel commented Jun 28, 2019

Rebased PR to master.

Refactoring the shelffile from a monolithic blob to sqlite3 db.
Advantages over pickle:
- Fast
- Small (25% of fill_address_space code)
- Not dependent on Python2/3 version
- Supports transactional read/(write TODO)
- Supports real-time persistence of user address-space (TODO)
- Strong typed: only INTEGER, TEXT and opc-ua to_binary() BLOB
- Drop-in replacement for memory-based AddressSpace

Built-in integrity check on generate_address_space.py dump.
Multiprocessing causes locking error when write and commits
from multiple threads are interleaved. Make sure that commits
are performed under the same Lock() session.
Since the sqlite address-space is lazy loading,
the old approach with incremental identifiers is
very slow, as it requires loading the complete
address space in memory before the highest identifier
is found. Generating a random uid and checking if
it already exists in the address space is much faster,
especially for hunderds of nodes.
@brubbel
Copy link
Contributor Author

brubbel commented Dec 17, 2019

Rebased PR to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants